-
Notifications
You must be signed in to change notification settings - Fork 44
feat(keychain): request storage access on auth #2377
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Code ReviewI found 2 issues that need attention: 🔴 High Priority: Race condition - Storage access not awaited before authLocation: The controller/packages/keychain/src/components/connect/create/CreateController.tsx Lines 505 to 522 in a93bc41
const handleStorageAccessRequest = useCallback(() => {
if (!isIframe()) {
return;
}
void (async () => { // ⚠️ Fire-and-forget - nothing waits for this
try {
await requestStorageAccess();
} catch (error) {
console.error(
"[CreateController] Storage access request failed:",
error,
);
}
})();
}, []);All call sites follow this pattern: onStorageAccessRequest(); // Fires async operation
onSubmit(option); // Proceeds immediatelyThis creates a race condition where authentication proceeds before storage access is granted. According to the Storage Access API spec, the storage grant only takes effect after Suggested fix: Make const handleStorageAccessRequest = useCallback(async () => {
if (!isIframe()) {
return;
}
try {
await requestStorageAccess();
} catch (error) {
console.error(
"[CreateController] Storage access request failed:",
error,
);
}
}, []);
// Then at call sites:
await onStorageAccessRequest();
onSubmit(option);🟡 Low Priority: Storage access requested before guard checkLocation: The controller/packages/keychain/src/components/connect/create/CreateController.tsx Lines 201 to 209 in a93bc41
onSubmit={(e) => {
e.preventDefault();
onStorageAccessRequest(); // Called unconditionally
// Don't submit if dropdown is open
if (isDropdownOpen) {
return; // But form doesn't submit
}
// ...This means the storage access prompt may be shown to users even when the form won't actually submit (when the dropdown is open). This is a minor UX issue. Suggested fix: Move the storage access request after the guard check: onSubmit={(e) => {
e.preventDefault();
// Don't submit if dropdown is open
if (isDropdownOpen) {
return;
}
onStorageAccessRequest();
// ...Review Summary: Checked for bugs and CLAUDE.md compliance. Found 1 high priority race condition and 1 low priority UX issue. |
No description provided.